[web_request] add handling of client_max_size in request body #1414

pawelmhm · 2016-11-20T14:18:07Z

What do these changes do?

Adding size limits to request body so that malicious client can't cause trouble by trying to upload request with extremely big body.

default limit set to value same as in nginx 1mb;
if client tries to upload body that exceeds this limit it will get 413 HTTP Error;
limit can be updated by passing client_max_size argument to Application object

Are there changes in behavior for the user?

Yes, too big uploads will raise HTTP 413 error.

Related issue number

#1133

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
Add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new entry to CHANGES.rst
- Choose any open position to avoid merge conflicts with other PRs.
- Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

* default limit set to value same as in nginx 1mb; * if client tries to upload body that exceeds this limit it will get 413 HTTP Error; * limit can be updated by massing client_max_size argument to Application object

codecov-io · 2016-11-20T15:24:38Z

Codecov Report

Merging #1414 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1414      +/-   ##
==========================================
+ Coverage   96.61%   96.61%   +<.01%     
==========================================
  Files          30       30              
  Lines        7293     7298       +5     
  Branches     1268     1269       +1     
==========================================
+ Hits         7046     7051       +5     
  Misses        149      149              
  Partials       98       98

Impacted Files	Coverage Δ
aiohttp/web_reqrep.py	`98.75% <100%> (ø)`	✅
aiohttp/test_utils.py	`99.63% <100%> (ø)`	✅
aiohttp/web.py	`98.75% <100%> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b62229...98086dc. Read the comment docs.

pawelmhm · 2016-11-20T15:42:48Z

I stumbled on two issues here:

there are aiohttp.web_exceptions and aiohttp.errors. Difference between the two is slightly unclear. They seem to contain similar code (e.g. 400, or 405 exception definittions) It would be nice to add some docstrings explaining their purposes. It seems like aiohttp.errors are mostly internal errors raised by library, and web_exceptions are intended to be raised from client code, is that correct? I added 413 to errors, is that ok? I cant add it to web_exceptions because of circular imports.
web_server.RequestHandler.handle_request will fail with UnboundLocalVariable error in finally block (line 69) on every exception that is not HTTPException. It looks like a bug, am I right? self._handler() call in try part can raise multiple things that are not HTTPExceptions? It seems this was introduced recently, I did some change in code last weekend and behavior was different. Anyway I 'fixed' that, but without test, because I'm not exactly sure about this. Let me know if we should consider this bug or feature.

I guess code-cow failing because of missing test for 2.

asvetlov

At first thank you for PR very much.
It worth to be included after some corrections.

Actually aiohttp.errors is for internal and client use only .

I'd hope to get rid of it at all sometimes -- HttpProcessingError and family is really confusing.

Server side should use aiohttp.web_exceptions but end user code should never import it directly: from aiohttp.web_exceptions import HTTPMethodNotAllowed is ok for aiohttp itself but outer code should do from aiohttp.web import HTTPMethodNotAllowed.

Server code should catch aiohttp.web_exceptions but all other exceptions must raise 500 (it's done by server.py now`).

web_server.py API is unstable. It was not released yet.
The purpose of the API is introducing much more user friendly way for working with HTTP low-level server (operate with handy Request/Response classes but don't relay on web.Application, routes etc).

asvetlov · 2016-11-20T18:53:01Z

aiohttp/errors.py

@@ -122,6 +122,11 @@ class HttpBadRequest(BadHttpMessage):
    message = 'Bad Request'


+class HttpRequestEntityTooLarge(HttpProcessingError):


Please drop it. The class what you need is already exists and called aiohttp.web.HTTPRequestEntityTooLarge.

asvetlov · 2016-11-20T18:55:14Z

aiohttp/web.py

@@ -32,7 +32,8 @@ class Application(MutableMapping):

    def __init__(self, *, logger=web_logger, loop=None,
                 router=None,
-                 middlewares=(), debug=...):
+                 middlewares=(), debug=...,
+                 client_max_size=None):


Just use 1MB (1024*1024) as default value.

asvetlov · 2016-11-20T18:56:19Z

aiohttp/web_reqrep.py

@@ -49,10 +50,13 @@ class BaseRequest(collections.MutableMapping, HeadersMixin):

    POST_METHODS = {hdrs.METH_PATCH, hdrs.METH_POST, hdrs.METH_PUT,
                    hdrs.METH_TRACE, hdrs.METH_DELETE}
+    # Maximum allowed size of request body.
+    _CLIENT_MAX_SIZE = 1024**2


Drop it. The value should be either passed by caller or None if caller don't care.
No default for it please.

asvetlov · 2016-11-20T19:05:10Z

aiohttp/web_reqrep.py

+        if client_max_size:
+            max_size = client_max_size
+        else:
+            max_size = self._CLIENT_MAX_SIZE


max_size could be None if user operates with low-level API (without web.Application, routers, signals, middlewares and other related stuffs).

asvetlov · 2016-11-20T19:06:09Z

aiohttp/web_reqrep.py

@@ -363,6 +372,8 @@ def read(self):
            while True:
                chunk = yield from self._payload.readany()
                body.extend(chunk)
+                if len(body) >= self._client_max_size:


_client_max_size could be None

asvetlov · 2016-11-20T19:09:32Z

aiohttp/web_reqrep.py

@@ -363,6 +372,8 @@ def read(self):
            while True:
                chunk = yield from self._payload.readany()
                body.extend(chunk)
+                if len(body) >= self._client_max_size:
+                    raise HttpRequestEntityTooLarge()


For this particular case from .web_exceptions import HTTPRequestEntityTooLarge is allowed evil.
Yes, I know the import from function has ugly adore.
But I'm ok with this for the case.

asvetlov · 2016-11-20T19:11:23Z

aiohttp/web_server.py

@@ -53,17 +54,21 @@ def handle_request(self, message, payload):

        request = self._request_factory(message, payload, self)
        self._request = request
+        resp = None


Yes, it's a bug. Thank you for finding.

asvetlov · 2016-11-20T19:12:40Z

aiohttp/web_server.py


        try:
            try:
                resp = yield from self._handler(request)
            except HTTPException as exc:
                resp = exc
+            except HttpProcessingError as exc:


All exceptions not derived from HttpProcessingError should raise 500 Internal Server Errror (now it's done by server.py code).

asvetlov · 2016-11-20T19:14:32Z

aiohttp/web_server.py


            resp_msg = yield from resp.prepare(request)
            yield from resp.write_eof()
        finally:
-            resp._task = None
+            if hasattr(resp, "_task"):


If you'll use web_exceptions this code is not needed.

fafhrd91 · 2017-02-07T16:31:51Z

@pawelmhm are you still interested in working on this PR?

AraHaan · 2017-02-16T19:02:32Z

Uh @pawelmhm you might want to backup some of your changes and go with the latest commit on the master branch and work from there and remake this PR after that so that way it can fix the merge conflicts easier. Also it would be a good idea to also apply the requested changes that @asvetlov said.

pawelmhm · 2017-02-16T19:06:34Z

hey @AraHaan @fafhrd91 sorry for delay I was bit busy, yeah I'll do rebase against master and apply suggestions from @asvetlov I'll try to do this over the weekend

fafhrd91 · 2017-02-16T19:08:01Z

Cool, thanks

* use web_exceptions.HTTPRequestEntityTooLarge instead of same exceptions from errors * allow limit to be None, if limit is None dont perform this check

fafhrd91 · 2017-02-18T15:08:13Z

aiohttp/web_reqrep.py

+                if self._client_max_size \
+                        and len(body) >= self._client_max_size:
+                    # local import to avoid circular imports
+                    from aiohttp import web_exceptions


Try to import aiohttp.web. All web exceptions are available i that namedspace

ah I see you merged that, I can check if import of aiohttp.web works, should I open new PR with that + doc updates, adding myself to contributors?

I fixed import and added you to contributors already. thanks

fafhrd91 · 2017-02-18T16:46:32Z

thanks

lock · 2019-10-29T04:04:09Z

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

pawelmhm force-pushed the max_size-1133 branch from 3a56b4e to 0da4166 Compare November 20, 2016 14:24

[web_request] add handling of client_max_size

b498200

* default limit set to value same as in nginx 1mb; * if client tries to upload body that exceeds this limit it will get 413 HTTP Error; * limit can be updated by massing client_max_size argument to Application object

pawelmhm force-pushed the max_size-1133 branch from 0da4166 to b498200 Compare November 20, 2016 14:38

use aiohttp.errors instead of aiohttp.web_exceptions when handling 413

b2b8516

asvetlov requested changes Nov 20, 2016

View reviewed changes

fafhrd91 force-pushed the master branch 4 times, most recently from 75f87f5 to be930a4 Compare February 9, 2017 20:34

fafhrd91 force-pushed the master branch from b2703b7 to 0068db9 Compare February 17, 2017 20:52

pawelmhm added 2 commits February 18, 2017 11:53

Merge branch 'master' into max_size-1133

bd2493e

[client_max_size] use web_exceptions, allow for limit to be None

98086dc

* use web_exceptions.HTTPRequestEntityTooLarge instead of same exceptions from errors * allow limit to be None, if limit is None dont perform this check

fafhrd91 reviewed Feb 18, 2017

View reviewed changes

fafhrd91 merged commit 1819363 into aio-libs:master Feb 18, 2017

lock bot added the outdated label Oct 29, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[web_request] add handling of client_max_size in request body #1414

[web_request] add handling of client_max_size in request body #1414

pawelmhm commented Nov 20, 2016 •

edited

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

pawelmhm commented Nov 20, 2016 •

edited

Loading

asvetlov left a comment

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

asvetlov Nov 20, 2016

fafhrd91 commented Feb 7, 2017

AraHaan commented Feb 16, 2017 •

edited

Loading

pawelmhm commented Feb 16, 2017

fafhrd91 commented Feb 16, 2017

fafhrd91 Feb 18, 2017

pawelmhm Feb 18, 2017 •

edited

Loading

fafhrd91 Feb 18, 2017

fafhrd91 commented Feb 18, 2017

lock bot commented Oct 29, 2019

		@@ -122,6 +122,11 @@ class HttpBadRequest(BadHttpMessage):
		message = 'Bad Request'


		class HttpRequestEntityTooLarge(HttpProcessingError):

[web_request] add handling of client_max_size in request body #1414

[web_request] add handling of client_max_size in request body #1414

Conversation

pawelmhm commented Nov 20, 2016 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov-io commented Nov 20, 2016 • edited Loading

Codecov Report

pawelmhm commented Nov 20, 2016 • edited Loading

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fafhrd91 commented Feb 7, 2017

AraHaan commented Feb 16, 2017 • edited Loading

pawelmhm commented Feb 16, 2017

fafhrd91 commented Feb 16, 2017

Choose a reason for hiding this comment

pawelmhm Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fafhrd91 commented Feb 18, 2017

lock bot commented Oct 29, 2019

pawelmhm commented Nov 20, 2016 •

edited

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

pawelmhm commented Nov 20, 2016 •

edited

Loading

AraHaan commented Feb 16, 2017 •

edited

Loading

pawelmhm Feb 18, 2017 •

edited

Loading